Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Keep dynamic patterns in their own groups #317

Closed
wants to merge 1 commit into from

Conversation

vladdu
Copy link

@vladdu vladdu commented Jun 19, 2021

If using multiple dynamic patterns where some point out to ../ and others
would be in the '.' group, all are added to this group, but the file
walker can't get out of the base directory and not all files are found.

What is the purpose of this pull request?

Addresses #316

What changes did you make? (Give an overview)

Removed the '.' task group, as it can't contain patterns that point to places outside the base directory.

It would be possible to solve in a different way, by looking for ../ patterns, but I'm not sure if there are more cases where that could be a problem. Is having multiple task groups going to be a performance problem?

I did not create tests, as I didn't find any that work against a list of patterns. Maybe I missed them?

If using multiple dynamic patterns where some point out to ../ and others
would be in the '.' group, all are added to this group, but the file
walker can't get out of the base directory and not all files are found.
@mrmlnc
Copy link
Owner

mrmlnc commented Jun 22, 2021

Hello, @vladdu,

Thanks for the PR.

Unfortunately, I think this solution only works well in your specific case. Without combining patterns and when we have something like this (['./**/*.js', '{a,b}/*.md']) we will read from the FS several times. The current division is designed to organize the reading in the most optimal way.

I will close this PR and suggest continuing in the issue.

@mrmlnc mrmlnc closed this Jun 22, 2021
@vladdu
Copy link
Author

vladdu commented Jun 22, 2021

I see. Ok, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants